-
Notifications
You must be signed in to change notification settings - Fork 550
Respect original variable type when using extract on optional keys #4450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.1.x
Are you sure you want to change the base?
Conversation
64793f6 to
9f4771a
Compare
9f4771a to
a3eb7f9
Compare
|
|
||
| use function PHPStan\Testing\assertType; | ||
|
|
||
| /** @return array{x: string, y?: string} */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add z?: string here and test it without assigning first.
I'd say that if $scope->canAnyVariableExist() is true and variable certainty is maybe, $z has to be mixed after extract.
The script can be included from somewhere where $z is already defined and is of a different type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| assertType('null', $x); | ||
| assertType('null', $y); | ||
| extract(foo()); | ||
| assertType('string', $x); | ||
| assertType('string|null', $y); // <-- should be: null|string | ||
| assertType('mixed', $z); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would also make sense to assertVariableCertainty on the 3 vars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
Closes phpstan/phpstan#12364